-
Notifications
You must be signed in to change notification settings - Fork 108
feat(flashblocks): add UnifiedReceiptBuilder for seamless deposit receipt handling #345
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🟡 Heimdall Review Status
|
| input, | ||
| &mut self.l1_block_info, | ||
| ) | ||
| .unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we properly bubble up an error here instead of using .unwrap()? We should have fixed this previously but if we're modifying this, let's cleanup past issues.
| self.cumulative_gas_used, | ||
| self.pending_block.timestamp, | ||
| ) | ||
| .map_err(|_| ExecutionError::DepositAccountLoad)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's implement From<ReceiptBuildError> for ExecutionError. This should let us remove the .map_err here and just use the ? operator.
So this would just become:
let receipt = self
.receipt_builder
.build(
&mut self.evm,
&transaction,
result,
&state,
self.cumulative_gas_used,
self.pending_block.timestamp,
)?;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| evm: &mut E, | ||
| transaction: &Recovered<OpTxEnvelope>, | ||
| result: ExecutionResult<E::HaltReason>, | ||
| _state: &EvmState, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this argument if it's unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
refcell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few changes + nits
|
@refcell Ready to review |
| impl From<crate::ReceiptBuildError> for ExecutionError { | ||
| fn from(_: crate::ReceiptBuildError) -> Self { | ||
| Self::DepositAccountLoad | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be the ExecutionError::RpcReceiptBuild() error variant rather than the ExecutionError::DepositAccountLoad unless I'm not understanding something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you are absolutly right
| #[test] | ||
| fn test_legacy_receipt_type() { | ||
| let tx = create_legacy_tx(); | ||
| assert_eq!(tx.tx_type(), OpTxType::Legacy); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_deposit_receipt_type() { | ||
| let tx = create_deposit_tx(); | ||
| assert_eq!(tx.tx_type(), OpTxType::Deposit); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm
| #[test] | |
| fn test_legacy_receipt_type() { | |
| let tx = create_legacy_tx(); | |
| assert_eq!(tx.tx_type(), OpTxType::Legacy); | |
| } | |
| #[test] | |
| fn test_deposit_receipt_type() { | |
| let tx = create_deposit_tx(); | |
| assert_eq!(tx.tx_type(), OpTxType::Deposit); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| #[test] | ||
| fn test_tx_type_to_receipt_mapping() { | ||
| let receipt = | ||
| Receipt { status: Eip658Value::Eip658(true), cumulative_gas_used: 21000, logs: vec![] }; | ||
|
|
||
| // Test all non-deposit variants | ||
| assert!(matches!(OpReceipt::Legacy(receipt.clone()), OpReceipt::Legacy(_))); | ||
| assert!(matches!(OpReceipt::Eip2930(receipt.clone()), OpReceipt::Eip2930(_))); | ||
| assert!(matches!(OpReceipt::Eip1559(receipt.clone()), OpReceipt::Eip1559(_))); | ||
| assert!(matches!(OpReceipt::Eip7702(receipt), OpReceipt::Eip7702(_))); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a no-op test, rm.
| #[test] | |
| fn test_tx_type_to_receipt_mapping() { | |
| let receipt = | |
| Receipt { status: Eip658Value::Eip658(true), cumulative_gas_used: 21000, logs: vec![] }; | |
| // Test all non-deposit variants | |
| assert!(matches!(OpReceipt::Legacy(receipt.clone()), OpReceipt::Legacy(_))); | |
| assert!(matches!(OpReceipt::Eip2930(receipt.clone()), OpReceipt::Eip2930(_))); | |
| assert!(matches!(OpReceipt::Eip1559(receipt.clone()), OpReceipt::Eip1559(_))); | |
| assert!(matches!(OpReceipt::Eip7702(receipt), OpReceipt::Eip7702(_))); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| #[test] | ||
| fn test_build_legacy_receipt() { | ||
| let chain_spec = Arc::new(OpChainSpecBuilder::base_mainnet().build()); | ||
| let mut db = InMemoryDB::default(); | ||
| let mut evm = create_test_evm(chain_spec.clone(), &mut db); | ||
|
|
||
| let builder = UnifiedReceiptBuilder::new(chain_spec); | ||
| let tx = create_legacy_tx(); | ||
| let result = create_success_result(); | ||
|
|
||
| let receipt = builder.build(&mut evm, &tx, result, 21000, 0).expect("build should succeed"); | ||
|
|
||
| assert!(matches!(receipt, OpReceipt::Legacy(_))); | ||
| if let OpReceipt::Legacy(inner) = receipt { | ||
| assert!(inner.status.coerce_status()); | ||
| assert_eq!(inner.cumulative_gas_used, 21000); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. A couple of nits. Will add @meyer9 as a reviewer to get another pair of eyes since this addresses a ticket he wrote.
You'll also need to resolve conflicts with trunk
meyer9
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG other than the comments @refcell left! Will let them approve once comments are addressed!
…eipt handling Add a new UnifiedReceiptBuilder that handles both deposit and non-deposit transactions seamlessly without requiring error handling at the call site. The builder wraps OpRethReceiptBuilder and automatically handles the deposit receipt case internally, eliminating the need for callers to implement the try-catch pattern typically required when using build_receipt followed by build_deposit_receipt. Changes: - Add receipt_builder module with UnifiedReceiptBuilder - Update PendingStateBuilder to use UnifiedReceiptBuilder - Simplify execute_with_evm method by removing manual deposit handling - Remove receipt_builder parameter from PendingStateBuilder::new() Closes base#309
Removed wrapper around OpRethReceiptBuilder and implemented direct receipt building logic. This fixes type mismatches between OpTxEnvelope and OpTransactionSigned that prevented compilation. - Use direct OpTxType matching instead of build_receipt/build_deposit_receipt pattern - Accept ExecutionResult<E::HaltReason> for proper generic type handling - Remove unused alloy-op-evm dependency
- Add From<ReceiptBuildError> for ExecutionError and StateProcessorError - Remove unused _state parameter from build() - Replace .unwrap() with proper error handling - Add comprehensive tests for receipt building logic
- Add EVM-based tests that directly test build() method - Test legacy receipt building - Test deposit receipt building - Test Canyon hardfork activation (deposit_receipt_version) - Test failed transaction receipt
- Use RpcReceiptBuild error variant in From<ReceiptBuildError> impl - Remove no-op tests (test_legacy_receipt_type, test_deposit_receipt_type, test_tx_type_to_receipt_mapping)
090c618 to
a24dbfe
Compare
|
@refcell I think it's ready now |
Summary
Introduces
UnifiedReceiptBuilder- a wrapper that handles both deposit and non-deposit transaction receipts seamlessly, eliminating the try-catch pattern required byOpReceiptBuilder.Changes
receipt_builder.rswithUnifiedReceiptBuilder<C>structbuild()method handles both tx types internallystate_builder.rs- removed ~40 lines of manual deposit handlingalloy-op-evmdependencyCloses #309